package db tests #23
package db tests #23
Conversation
cmd/set.go
Outdated
| cmd.SilenceUsage = true | ||
| return errors.New("Not implemented yet") | ||
| fmt.Println("Not implemented yet") | ||
| return ErrFlagNotImplemented |
There was a problem hiding this comment.
I don't understand this change
Using fmt.Println won't send message to stderr but stdout
Is the cause the fact you are using testify assert.Equal(t, ErrFlagNotImplemented, err) ?
The errors were wrapped now, they are reported unwrapped
cmd/set.go
Outdated
| ErrFlagNotImplemented error = errors.New("flag not implemented yet") | ||
| ErrBadUsageSetCmd error = errors.New("bad usage of set command") |
There was a problem hiding this comment.
There is no need to define the type to error here
cmd/root.go
Outdated
| StatusBucket string = "projects" | ||
| ProjectPathBucket string = "projectPaths" | ||
| ProjectAliasBucket string = "projectAliases" | ||
| version string = "1.0" |
There was a problem hiding this comment.
There is no need to define the type string here.
cmd/ls.go
Outdated
| oldUi, _ := cmd.Flags().GetBool("o") | ||
| data, err := db.GetAllRecords(StatusBucket) | ||
| data, err := db.GetAllRecords(db.DBName, StatusBucket) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if filterFlag != "" { | ||
| fmt.Println("Filtering by status : ", filterFlag) | ||
| data = utils.FilterByStatus(data, filterFlag) | ||
| data := utils.FilterByStatus(data, filterFlag) | ||
| ui.RenderTable(data) | ||
| } | ||
| if oldUi { | ||
| return ui.RenderTable(data) | ||
| } |
There was a problem hiding this comment.
You are removing everything about oldui, I don't understand.
It might be legitimate but I don't get it
pkg/db/db_test.go
Outdated
| }) | ||
|
|
||
| t.Run("Test WriteToDB with empty bucketname", func(t *testing.T) { | ||
| dbname := db.DBTestName |
There was a problem hiding this comment.
You are defining it in almost every single test. Add a const at the beginning of _test package and stop redefining again and again
There was a problem hiding this comment.
You are talking about what in this part ? i don't get it
There was a problem hiding this comment.
adding a const dbname := db.DBTestName at the top of the package
so you don't have to repeat it everywhere. And when you need to set it to empty string, as I think it's the only other use case, you can then use either another variable or using dbname := ""
pkg/db/db_test.go
Outdated
| require.Error(t, err) | ||
| require.ErrorIs(t, err, db.ErrWriteToDB) |
There was a problem hiding this comment.
ErrorIs implies Error
So no need to use Error here
pkg/db/db_test.go
Outdated
|
|
||
| require.Error(t, err) | ||
| require.ErrorIs(t, err, expectedErr) | ||
| require.Equal(t, records, expectedValue) |
There was a problem hiding this comment.
Equal(t, expected, actual)
You inverted
pkg/db/db_test.go
Outdated
| require.Error(t, err) | ||
| require.Equal(t, expectedErr, err) |
There was a problem hiding this comment.
Please use
require.ErrorIs(t, err, expectedErr)
pkg/indexer.go
Outdated
| fmt.Println(err) | ||
| return ErrIndexDir |
There was a problem hiding this comment.
No fmt.Println should be called.
Using this would be better
return fmt.Errorf("%w: %w")Or simply a join
|
I would ask you to wait before #27 to be merged if you are OK guys it would report CI errors on your code |
|
@theredditbandit you asked me to review, but I will wait for the CI changes to be rebased in this branch, so we could see what the CI will report |
|
I will need to be in front of a computer to review in details, but I'm impressed by the changes you made @yannick2009 |
|
@ccoVeille can you figure out why ci is breaking , I can't make sense of the error message. |
|
Finally all checks pass! 🎉 To get rid of the error I had to run to format it like golangci-lint likes it @ccoVeille did you have any thoughts on these changes |
Launch
go test -cover ./...to run the tests and see the percentage of coverage.